Implementation for rmw_get_pub/sub_info_by_topic #97

dpotman · 2020-02-06T10:32:28Z

Implementation for the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods, and some related refactoring:

rewrite rmw_collect_tptyp_for_kind to the more generic rmw_collect_data_for_endpoint
moved code from rmw_get_topic_endpoint_info.cpp to rmw_node.cpp (restructuring of rmw_node.cpp file is planned, but not in this PR)
extracted qos conversion function dds_qos_to_rmw_qos from get_readwrite_qos

When this PR is merged, the rcl test test_rcl_get_publishers_subscription_info_by_topic can be enabled for Cyclone; I'll create a PR for this.

Implementation for the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods. Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>

dpotman · 2020-02-06T10:35:54Z

This PR resolves issue #82 (may need refactoring in case the Participant mapping is moved to Context)

eboasson

Thanks @dennis-adlink for this pull request. As I want to see the Cyclone RMW layer complete, I decided to respond to @ivanpauno's request in #87. It's really mostly nit-picking, the only mistake is in the error handling in get_endpoint_info_by_topic, but I also think there might be a problem in the underlying rmw interfaces ...

eboasson · 2020-02-07T10:29:41Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+  dds_qos_t * qos = dds_create_qos();
+  if (dds_get_qos(handle, qos) < 0) {
+    RMW_SET_ERROR_MSG("get_readwrite_qos: invalid handle");
+    goto error;


I would suggest removing the goto errors here, considering that only the case of dds_get_qos returning an error needs some error handling.

eboasson · 2020-02-07T10:38:28Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

    return RMW_RET_ERROR;
  }
  dds_sample_info_t info;
  void * msg = NULL;
  int32_t n;
-  while ((n = dds_take(rd, &msg, &info, 1, 1)) == 1) {
+  bool cont = true;


All filter/map functions passed to rmw_collect_data_for_endpoint always return true, and so the cont variable isn't needed. I would suggest removing the return value.

eboasson · 2020-02-07T10:42:39Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+typedef std::map<dds_builtintopic_guid_t, std::pair<std::string, std::string>> node_cache_t;
+
+static rmw_ret_t get_node_name(
+  dds_entity_t rd,


Perhaps it would be better to indicate more clearly that rd must be a reader of the "participant" built-in topic — and possible also to assert that is. A name change would do the trick, ppant_rd could be an option (but I find it hard to guess how much confusion it would eliminate or create for readers that don't know Cyclone well).

eboasson · 2020-02-07T10:44:53Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+static rmw_ret_t
+set_rmw_topic_endpoint_info(
+  node_cache_t & node_cache,
+  dds_entity_t rd,


See comment concerning rd in get_node_name — the same applies here.

eboasson · 2020-02-07T10:48:07Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+    node_name = "_NODE_NAME_UNKNOWN_";
+    node_ns = "_NODE_NAMESPACE_UNKNOWN_";
+  }
+  return RMW_RET_OK;


Not returning a value would eliminate the return value checks later on.

eboasson · 2020-02-07T10:51:22Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+  {
+    return ret;
+  }
+  topic_endpoint_info.qos_profile = epinfo.qos_profile;


Shouldn't this be rmw_topic_endpoint_info_set_qos_profile to continue the pattern?

eboasson · 2020-02-07T10:54:44Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+            "/");
+          info.topic_type = std::string(demangled_type) + std::string(cm_typ[2]);
+        } else {
+          return true;


Perhaps add a comment that this skips a non-ROS2 endpoint?

eboasson · 2020-02-07T10:57:49Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+    (rd = dds_create_reader(
+      node_impl->enth, DDS_BUILTIN_TOPIC_DCPSPARTICIPANT, NULL, NULL)) < 0)
+  {
+    RMW_SET_ERROR_MSG("get_endpoint_info_by_topic: failed to create reader");
    return RMW_RET_ERROR;


I think this is missing a call to rmw_topic_endpoint_info_array_fini because the init_with_size function before allocated memory. However, I'm not entirely certain of this because array_fini invokes endpoint_info_fini on its elements and I don't see a guarantee the array init_with_size allocates is properly initialized.

If you first call get_zero_initialized_topic_endpoint_info_array, and then init_with_size, it shouldn't be a problem to call rmw_topic_endpoint_info_array_fini when something fails.

eboasson · 2020-02-07T10:58:17Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+        handle_topic_endpoint_info_fini(&endpoint_info->info_array[n], allocator);
+      }
+      dds_delete(rd);
+      return ret;


See the comment above

eboasson · 2020-02-07T11:08:31Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+  rmw_topic_endpoint_info_array_t * endpoint_info)
+{
+  auto node_impl = static_cast<CddsNode *>(node->data);
+  const auto re_typ = std::regex("^(.*::)dds_::(.*)_$", std::regex::extended);


I think it would make sense to create a function to demangle topic names/types, with a parameter specifying whether it concerns data, requests or responses. There was some duplication already, but this adds yet another one and having regexes all over the place is a bit unpleasant.

…fixed enpoint_info_array_fini and a few other minor fixes Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>

dpotman · 2020-02-08T10:15:17Z

@eboasson Thanks for the review! I fixed the issues in my latest commit:

introduced a function for topic name/type demangle
renamed rd to ppant_rd
fixed cleaning up topic_endpoint_info_array in case of failure
removed return value from get_node_name function
refactored goto's in get_readwrite_qos
removed cont from rmw_collect_data_for_endpoint

Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>

dpotman · 2020-02-08T10:41:04Z

One more commit to refactor out the extra call to demangle_topic_type in get_endpoint_names_and_types_by_node

Signed-off-by: Erik Boasson <eb@ilities.com>

eboasson · 2020-02-10T12:52:02Z

Thanks @dennis-adlink. The only thing was a warning for an unused argument in a release build, I've pushed a fix for that.

@ivanpauno I think it can be merged, but perhaps you would like to have a quick look or do a CI run first?

ivanpauno · 2020-02-10T13:04:52Z

@ivanpauno I think it can be merged, but perhaps you would like to have a quick look or do a CI run first?

Yes, I will run CI together with the rcl PR, so we check that tests are passing.

ivanpauno

I think error handling should be fixed before merging, otherwise LGTM!

ivanpauno · 2020-02-10T15:30:26Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+    (rd = dds_create_reader(
+      node_impl->enth, DDS_BUILTIN_TOPIC_DCPSPARTICIPANT, NULL, NULL)) < 0)
+  {
+    RMW_SET_ERROR_MSG("get_endpoint_info_by_topic: failed to create reader");
    return RMW_RET_ERROR;


If you first call get_zero_initialized_topic_endpoint_info_array, and then init_with_size, it shouldn't be a problem to call rmw_topic_endpoint_info_array_fini when something fails.

dpotman · 2020-02-10T16:42:50Z

I think error handling should be fixed before merging, otherwise LGTM!

rmw_topic_endpoint_info_array_init_with_size does not set count, so that's why I initialize this to zero. After that point I use rmw_topic_endpoint_info_array_fini for cleaning up, also in case of an error (note that the code fragments above your remark is outdated). So I think error handling is properly in now.

ivanpauno · 2020-02-10T17:56:35Z

rmw_topic_endpoint_info_array_init_with_size does not set count, so that's why I initialize this to zero

I'm going to send a PR to fix that, because it's confusing (and incongruent with other arrays defined in rcutils/rmw). We can iterate on that in another PR, when the change in rmw_topic_endpoint_info_array_init_with_size is done.

After that point I use rmw_topic_endpoint_info_array_fini for cleaning up, also in case of an error (note that the code fragments above your remark is outdated). So I think error handling is properly in now.

Sounds good!

Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>

Implementation for rmw_get_pub/sub_info_by_topic

2fdccda

Implementation for the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods. Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>

dpotman mentioned this pull request Feb 6, 2020

Fix test_rcl_get_publishers_subscription_info_by_topic ros2/rcl#571

Merged

ivanpauno mentioned this pull request Feb 6, 2020

Implement the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods #82

Closed

eboasson reviewed Feb 7, 2020

View reviewed changes

dpotman mentioned this pull request Feb 7, 2020

Enable tests for get_pub/sub_info_by_topic for Cyclone DDS ros2/rcl#572

Merged

Processed review comments: moved topic/type demangling to functions, …

e04454c

…fixed enpoint_info_array_fini and a few other minor fixes Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>

Some additional refactoring in get_endpoint_names_and_types_by_node

b06b6b0

Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>

Fix "unused argument" warning in release builds

2bf02d2

Signed-off-by: Erik Boasson <eb@ilities.com>

eboasson approved these changes Feb 10, 2020

View reviewed changes

ivanpauno reviewed Feb 10, 2020

View reviewed changes

ivanpauno merged commit d70fad6 into ros2:master Feb 10, 2020

eboasson mentioned this pull request Feb 11, 2020

Fix dashing builds wrt rmw_topic_endpoint_info_t #99

Merged

eboasson pushed a commit that referenced this pull request Mar 12, 2020

Implementation for rmw_get_pub/sub_info_by_topic (#97)

ed96b80

Signed-off-by: Dennis Potman <dennis.potman@adlinktech.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation for rmw_get_pub/sub_info_by_topic #97

Implementation for rmw_get_pub/sub_info_by_topic #97

dpotman commented Feb 6, 2020

dpotman commented Feb 6, 2020

eboasson left a comment

eboasson Feb 7, 2020

eboasson Feb 7, 2020

eboasson Feb 7, 2020

eboasson Feb 7, 2020

eboasson Feb 7, 2020

eboasson Feb 7, 2020

eboasson Feb 7, 2020

eboasson Feb 7, 2020

ivanpauno Feb 10, 2020

eboasson Feb 7, 2020

eboasson Feb 7, 2020

dpotman commented Feb 8, 2020

dpotman commented Feb 8, 2020

eboasson commented Feb 10, 2020

ivanpauno commented Feb 10, 2020

ivanpauno left a comment

ivanpauno Feb 10, 2020

dpotman commented Feb 10, 2020

ivanpauno commented Feb 10, 2020

Implementation for rmw_get_pub/sub_info_by_topic #97

Implementation for rmw_get_pub/sub_info_by_topic #97

Conversation

dpotman commented Feb 6, 2020

dpotman commented Feb 6, 2020

eboasson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpotman commented Feb 8, 2020

dpotman commented Feb 8, 2020

eboasson commented Feb 10, 2020

ivanpauno commented Feb 10, 2020

ivanpauno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpotman commented Feb 10, 2020

ivanpauno commented Feb 10, 2020